Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

replace simple IP pool free table with LRU implementation #450

Merged
merged 1 commit into from
Oct 27, 2021

Conversation

RoadRunnr
Copy link
Member

The free table was effectively acting as a queue (LRU). push and
pop operation where fast, but a direct removal of a single item
did require a full table scan.
For large IP pools, that could lead to unacceptable delays.

The LRU implementation uses two indexes and is much faster when
removing an item from the middle of the queue.

The free table was effectively acting as a queue (LRU). push and
pop operation where fast, but a direct removal of a single item
did require a full table scan.
For large IP pools, that could lead to unacceptable delays.

The LRU implementation uses two indexes and is much faster when
removing an item from the middle of the queue.
@RoadRunnr RoadRunnr requested a review from a team as a code owner October 25, 2021 15:55
@javiermtorres
Copy link

Do we have metrics comparing the old implementation to this one?

Also, since authentication will fill in a value using the Framed-IP-Address, can a dummy pool be used returning a fixed IP that will always be overwritten by the RADIUS server?

@RoadRunnr
Copy link
Member Author

Do we have metrics comparing the old implementation to this one?

The old implementation achieves 4 ops/s on a /10 prefix, the new version about 500000 ops/s, and a maps and gb_trees based version about 200000 ops/s (all measured for the take operation).

Also, since authentication will fill in a value using the Framed-IP-Address, can a dummy pool be used returning a fixed IP that will always be overwritten by the RADIUS server?

There is nothing that dictates that a pool can not be shared between AAA based assignment and local assignment. Using a dummy pool is therefore not an option.

@javiermtorres
Copy link

Ok, I'm not sure I follow the pool config. So we would have a pool (a collection of prefixed IP addresses) that would be shared among AAA and local. I assume AAA and local will not track each other's assignments, or do they? If they don't, can't they have separate, non overlapping pools covering the overall pool?

@RoadRunnr
Copy link
Member Author

Ok, I'm not sure I follow the pool config. So we would have a pool (a collection of prefixed IP addresses) that would be shared among AAA and local. I assume AAA and local will not track each other's assignments, or do they? If they don't, can't they have separate, non overlapping pools covering the overall pool?

The ergw pool tracks the IP assigned by AAA, that is whole purpose of the take operation (compared to the assign that gives out a somewhat random, free IP).

AAA gets told which session has which IP, so it can track the used IPs.

In a normal NAS (Network Access Server), e.g. Cisco BNG, nothing stops AAA to issue static IPs from a pool shared with the NAS as long as the IP is unused.

There is also no technical requirement to have seperate pool types for AAA and local assignments.

Also, the design decision to have a unified pool logic was made long ago. I don't see any need to question or revisit that in a PR that implements a performance optimization.

@javiermtorres
Copy link

Thanks for the detailed explanation. Understanding the rationale for design decisions is also important, and not obvious at first glance. Also, assumptions for design decisions sometimes change.

@RoadRunnr RoadRunnr merged commit 5e7d9ef into master Oct 27, 2021
@RoadRunnr RoadRunnr deleted the feature/new-lru branch October 27, 2021 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants